For Menu.wmTimer use Menu.indexOf instead of MenuItem.index #1575

merks · 2024-11-06T08:53:04Z

Remove MenuItem.index which is now unused.
Remove unused MenuItem.MenuItem(Menu, Menu, int, int)

eclipse-platform/eclipse.platform.ui#2483

merks · 2024-11-06T08:57:37Z

I wonder who could review this?

To reproduce the problem I used Navigate -> Open Setup -> Workspace and have this additional target, which you can copy and paste:

<?xml version="1.0" encoding="UTF-8"?>
<setup.targlets:TargletTask
    xmi:version="2.0"
    xmlns:xmi="http://www.omg.org/XMI"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xmlns:setup.targlets="http://www.eclipse.org/oomph/setup/targlets/1.0"
    xsi:schemaLocation="http://www.eclipse.org/oomph/setup/targlets/1.0 https://raw.githubusercontent.com/eclipse-oomph/oomph/master/setups/models/SetupTarglets.ecore">
  <targlet name="EGit">
    <requirement
        name="org.eclipse.egit.feature.group"/>
    <repositoryList>
      <repository
          url="https://download.eclipse.org/egit/updates-stable-nightly"/>
    </repositoryList>
  </targlet>
</setup.targlets:TargletTask>

Perform the targlet task and In the launch configuration include the EGit feature. Then in any plain text editor the Team submenu's items should all have tooltips that don't work without these changes because the index recorded in the MenuItem is simply wrong because dispose() is called on other menu items in the menu changing the index of other items

github-actions · 2024-11-06T09:04:43Z

Test Results

483 files ±0 483 suites ±0 8m 35s ⏱️ -11s
4 095 tests ±0 4 085 ✅ ±0 7 💤 ±0 3 ❌ ±0
16 173 runs ±0 16 080 ✅ ±0 90 💤 ±0 3 ❌ ±0

For more details on these failures, see this check.

Results for commit e0956b9. ± Comparison against base commit 8593132.

♻️ This comment has been updated with latest results.

merks · 2024-11-06T09:13:24Z

The build looks like a bit of a mess, but nothing looks related to my actual changes. Does anyone know what's going on?

HeikoKlare · 2024-11-06T13:23:01Z

I can try to find some time to have a look at the PR. Is it supposed to be in M3 or is targeted for the next development cycle?

The builds currently look like this because of browser test failures on Linux:

merks · 2024-11-07T01:57:41Z

@HeikoKlare

I would prefer we get this in for m3.

I don't actually think it needs any review. The existing design is just fundamentally broken because the index of a menu item changes when items are inserted or disposed. So recording the index at the time of creation is just wrong.

Note that it's not even necessary to reproduce the original problem but simply to confirm that tool tips do show with the new logic, which is the case:

I'll add some comments and if you wish to review them, that's fine. If not I will merge the changes later today.

merks

Note the only potentially controversial change is the removal of the unused MenuItem constructor.

merks · 2024-11-07T01:58:33Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

@@ -42,7 +42,7 @@
 public class MenuItem extends Item {
 	Menu parent, menu;
 	long hBitmap;
-	int id, accelerator, userId, index;


The index field was introduce when support for tooltips were added. I've eliminate the use/need for it, hence removed it.

merks · 2024-11-07T01:59:10Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

-	parent.createItem (this, (this.index = index));
-}
-
-MenuItem (Menu parent, Menu menu, int style, int index) {


When I remove the unused parameter I noticed that the entire constructor is unused so it seems better to remove it.

merks · 2024-11-07T02:00:13Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

@@ -136,16 +136,7 @@ public MenuItem (Menu parent, int style) {
 public MenuItem (Menu parent, int style, int index) {
 	super (parent, checkStyle (style));
 	this.parent = parent;
-	parent.createItem (this, (this.index = index));


This looks confusingly like a removall, but note there is addition below:

parent.createItem (this, index);

merks · 2024-11-07T02:01:03Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Menu.java

@@ -1349,7 +1349,7 @@ LRESULT wmTimer (long wParam, long lParam) {
 		OS.GetCursorPos (pt);
 		if (selectedMenuItem != null && selectedMenuItem.parent != null) {
 			RECT rect = new RECT ();
-			boolean success = OS.GetMenuItemRect (0, selectedMenuItem.parent.handle, selectedMenuItem.index, rect);
+			boolean success = OS.GetMenuItemRect (0, selectedMenuItem.parent.handle, indexOf(selectedMenuItem), rect);


The fundamental change here is to compute the correct index using the existing method for computing the index.

HeikoKlare

Thank you for all the helpful explanations on the code and on how to reproduce, Ed! I agree that a review would not have even been necessary here. Still, I have taken a look at the code and successfully tested the change. Everything looks good to me. Once (Jenkins) validation build succeeds, I will merge this.

Note the only potentially controversial change is the removal of the unused MenuItem constructor.

I don't think this is a controversial point as the constructor is obviously unused and not part of any public API.

- Remove MenuItem.index which is now unused. - Remove unused MenuItem.MenuItem(Menu, Menu, int, int) eclipse-platform/eclipse.platform.ui#2483

merks commented Nov 7, 2024

View reviewed changes

HeikoKlare force-pushed the issue-2483 branch from e474bea to ad1e109 Compare November 7, 2024 07:15

HeikoKlare approved these changes Nov 7, 2024

View reviewed changes

For Menu.wmTimer use Menu.indexOf instead of MenuItem.index

e0956b9

- Remove MenuItem.index which is now unused. - Remove unused MenuItem.MenuItem(Menu, Menu, int, int) eclipse-platform/eclipse.platform.ui#2483

HeikoKlare force-pushed the issue-2483 branch from ad1e109 to e0956b9 Compare November 7, 2024 08:23

merks merged commit e03196c into eclipse-platform:master Nov 7, 2024
8 of 14 checks passed

merks deleted the issue-2483 branch November 7, 2024 09:51

HeikoKlare mentioned this pull request Nov 7, 2024

Inconsistent display of menu tooltips eclipse-platform/eclipse.platform.ui#2483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For Menu.wmTimer use Menu.indexOf instead of MenuItem.index #1575

For Menu.wmTimer use Menu.indexOf instead of MenuItem.index #1575

merks commented Nov 6, 2024

merks commented Nov 6, 2024

github-actions bot commented Nov 6, 2024 •

edited

Loading

merks commented Nov 6, 2024

HeikoKlare commented Nov 6, 2024

merks commented Nov 7, 2024

merks left a comment

merks Nov 7, 2024

merks Nov 7, 2024

merks Nov 7, 2024

merks Nov 7, 2024

HeikoKlare left a comment

For Menu.wmTimer use Menu.indexOf instead of MenuItem.index #1575

For Menu.wmTimer use Menu.indexOf instead of MenuItem.index #1575

Conversation

merks commented Nov 6, 2024

merks commented Nov 6, 2024

github-actions bot commented Nov 6, 2024 • edited Loading

Test Results

merks commented Nov 6, 2024

HeikoKlare commented Nov 6, 2024

merks commented Nov 7, 2024

merks left a comment

Choose a reason for hiding this comment

merks Nov 7, 2024

Choose a reason for hiding this comment

merks Nov 7, 2024

Choose a reason for hiding this comment

merks Nov 7, 2024

Choose a reason for hiding this comment

merks Nov 7, 2024

Choose a reason for hiding this comment

HeikoKlare left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2024 •

edited

Loading